Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: support vyper files for tests #2684

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

tserg
Copy link
Collaborator

@tserg tserg commented Feb 26, 2022

What I did

How I did it

The Vyper files need to adhere to the syntax test_DESCRIPTION_OUTCOME where DESCRIPTION is non-consequential and OUTCOME is either Success, a VyperException class name, or Fail (unhandled exception). OUTCOME is extracted upon the initialisation of each Vyper test file.

For each test file, we attempt to compile it using compile_files, and check the compilation outcome against the extracted OUTCOME.

Example of a failing test:
Screenshot from 2022-02-26 17-26-40

How to verify it

Run pytest tests/parser/syntax/constants/ to see a demo against two sample contracts from test_constants.py. You can also edit the Vyper test files to see what a failing test look like.

Description for the changelog

Handle Vyper test files

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

@codecov-commenter
Copy link

codecov-commenter commented Feb 26, 2022

Codecov Report

Merging #2684 (68a2fd3) into master (3de52b3) will decrease coverage by 34.17%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #2684       +/-   ##
===========================================
- Coverage   86.29%   52.12%   -34.18%     
===========================================
  Files          91       91               
  Lines        9739     9737        -2     
  Branches     2466     2428       -38     
===========================================
- Hits         8404     5075     -3329     
- Misses        833     4061     +3228     
- Partials      502      601       +99     
Impacted Files Coverage Δ
vyper/builtin_interfaces/ERC20Detailed.py 0.00% <0.00%> (-100.00%) ⬇️
vyper/lll/s_expressions.py 5.88% <0.00%> (-88.24%) ⬇️
vyper/ast/natspec.py 12.34% <0.00%> (-86.42%) ⬇️
vyper/codegen/external_call.py 11.81% <0.00%> (-82.73%) ⬇️
vyper/codegen/types/convert.py 12.90% <0.00%> (-74.20%) ⬇️
vyper/cli/utils.py 16.66% <0.00%> (-71.43%) ⬇️
vyper/codegen/function_definitions/utils.py 28.57% <0.00%> (-71.43%) ⬇️
vyper/ast/expansion.py 23.52% <0.00%> (-70.59%) ⬇️
vyper/cli/vyper_json.py 9.38% <0.00%> (-70.40%) ⬇️
vyper/compiler/output.py 25.84% <0.00%> (-67.98%) ⬇️
... and 59 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3de52b3...68a2fd3. Read the comment docs.

Comment on lines 206 to 210
@pytest.mark.parametrize(
"vyper_contract", ["constants/test_constantGetter1_Success.vy"], indirect=True
)
def test_constant_success_2(vyper_contract):
assert vyper_contract.foo() == 123
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, this is clever. Basically, reuse compiled "successful parsing" test cases to additionally perform other checks?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's correct. This is more deliberate as compared to automatically creating fixtures for "successful parsing" test cases (which I still cannot figure out, and on further thought could be more confusing if all these contracts appear in the global namespace).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would the array of filenames passed in create multiple parametrized tests?

Also, some of the folders get really deep. I wonder if it could just assume a local path e.g. ./test_constantGetter1_Success.vy

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would the array of filenames passed in create multiple parametrized tests?

Yes, I have updated the example to better demonstrate this.

Also, some of the folders get really deep. I wonder if it could just assume a local path e.g. ./test_constantGetter1_Success.vy

Do you mean that there will be some sort of collection process in the current directory and subdirectories to get the Vyper files based on the filename provided?

@tserg tserg force-pushed the chore/direct_vyper_tests branch from 1ba2617 to 3ef9776 Compare February 28, 2022 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants